feat(x/ecocredit)!: implement independent projects #2167

aaronc · 2024-02-13T16:05:58Z

Description

Review instructions:

please check the CHANGELOG and modified .proto files first
to review new code, I suggest looking at the new .feature files first
changes to existing code have mostly involved removing the assumption that the class is implied by the project and that an enrollment is needed
you will need to look at some large diffs that github has hidden by default because they're so big, sorry

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

…onc/independent-projects

Co-authored-by: Cory <[email protected]>

…ojects-impl

x/ecocredit/base/client/tx.go

x/ecocredit/client/tx.go

Co-authored-by: Paul Weidner <[email protected]>

aaronc · 2024-07-09T10:32:02Z

@paul121 I believe I have addressed all of the simple changes you have suggested.

Regarding:

There is no way for a credit class to signal that they have "received" an application and are beginning review, maybe give an expected timeframe, etc. I assume this could happen via enrollment metadata.
The credit class can only add metadata when setting the status to ACCEPTED or CHANGES_REQUESTED.
Would it make sense to add a project enrollment status for "PENDING" / "IN REVIEW" so there is an intermediary status?

I think two straightforward ways are either:

allowing metadata to be added by the issuer in UNSPECIFIED state (without transitioning to ACCEPTED or CHANGES_REQUESTED)
or adding a PENDING state like you suggest

Either will involve adding some new test cases, but option 1 seems probably simpler to implement at this stage. Would that be okay or do you think there's a strong argument for a PENDING case? If we add a pending case would we allow transitioning from CHANGES_REQUESTED to PENDING?

paul121 · 2024-07-09T15:59:05Z

Either will involve adding some new test cases, but option 1 seems probably simpler to implement at this stage. Would that be okay or do you think there's a strong argument for a PENDING case? If we add a pending case would we allow transitioning from CHANGES_REQUESTED to PENDING?

I don't have a strong argument for a PENDING case. I agree option 1 makes sense for now, it's more general and flexible than adding any specific state. Chatting with @clevinson today he said there might be cases when a credit class doesn't want to reveal they are starting a formal review anyways, so yeah, let’s keep it simple for now, allowing metadata in the UNSPECIFIED state.

aaronc · 2024-07-10T10:23:52Z

@paul121 I updated UpdateProjectEnrollment to allow transitions from UNSPECIFIED or CHANGES_REQUESTED to UNSPECIFIED with updates to metadata in fd96f71. Please review that commit and let me know what you think.

paul121

@aaronc the changes to UpdateProjectEnrollment look good. Just noticing a test failure related to the CLI changes. Not sure if there are other integration tests to review as well

x/ecocredit/base/client/tx.go

paul121

Added a commit to fix one remaining integration test. I think this is good now.

x/ecocredit/base/keeper/msg_create_project.go

Co-authored-by: Paul Weidner <[email protected]>

clevinson

I've gone through all the code and generally looks good.

Handful of small test / comment related suggestions. In general I think we should lean towards using the new project ID's in all of our feature files / comments where possible, unless there's a specific reason to keep the legacy format.

One thing I'm a bit unclear on is how this PR handles creation of new batches for legacy projects (whether or not those batches are under a legacy credit class). By legacy here I simply mean a project or credit class created prior to this upgrade.

In the state migration, it looks like we don't update any existing Project ID's to the new format (as stored in the ORM table). So if a project (C01-001) has a pre-existing batch (C01-001-20210101-20220101-001), and then has a new batch issued after this update, I think the new batch would be C01-C01-001-20220101-20230101-002, since the new formatting of batch denom explicitly adds credit class ID to the beginning, even if project ID is in the legacy format.

Similarly, if this project enrolls in a second credit class, and creates a batch, then that batch would be C02-C01-001-20210101-20220101-001.

I understand why it's preferable to preserve legacy Project IDs and not change them, as that may break compatibility with certain RDF datasets, Sanity, or credit batch linkages. But are there any alternatives where we could maybe keep the legacy format for legacy enrollments, and generate a new format project ID only when a legacy project creates a new enrollment? Another alternative may be just not allowing legacy projects to have multiple enrollments at all.

clevinson · 2024-08-13T21:21:39Z

proto/regen/ecocredit/v1/query.proto

-  // ProjectEnrollments queries all credit class enrollments associated with a
-  // project.
+  // ProjectEnrollments queries all credit class enrollments and allows for filtering by
+  // project or credit class.
  rpc ProjectEnrollments(QueryProjectEnrollmentsRequest) returns (QueryProjectEnrollmentsResponse) {
    option (google.api.http) = {
      get : "/regen/ecocredit/v1/project/{project_id}/enrollments"


Is there a way for the GET request to support no project_id, such that it queries all project enrollments? Also, is there a way to optionally filter by class_id in the grpc-gateway path?

Would be nice to accommodate those use cases if possible.

proto/regen/ecocredit/v1/query.proto

x/ecocredit/base/client/tx.go

clevinson · 2024-08-13T23:46:10Z

x/ecocredit/base/keeper/features/msg_create_batch.feature

-      Then expect batch sequence with project id "C01-001" and next sequence "1"
+      Given a batch sequence with project id "P001" and next sequence "1"
+      When alice attempts to create a batch with project id "P002" and class id "C01"
+      Then expect batch sequence with project id "P001" and next sequence "1"


Suggested change

Then expect batch sequence with project id "P001" and next sequence "1"

Then expect batch sequence with project id "P002" and next sequence "1"

clevinson · 2024-08-15T19:07:57Z

x/ecocredit/base/keeper/query_projects_by_reference_id_test.go

@@ -33,14 +32,12 @@ func TestQuery_ProjectsByReferenceId(t *testing.T) {
 	assert.NilError(t, s.stateStore.ProjectTable().Insert(s.ctx, project))
 	assert.NilError(t, s.stateStore.ProjectTable().Insert(s.ctx, &api.Project{
 		Id:          "C01-002",


Suggested change

Id: "C01-002",

Id: "P002",

clevinson · 2024-08-15T19:19:32Z

x/ecocredit/base/types/v1/features/state_project.feature

    Given the project
    """
    {
      "key": 1,
-      "id": "foo"
+      "id": "C01-001"


Can project id's throughout this feature file be updated to the new format (e.g. P001)?

clevinson · 2024-08-15T21:21:51Z

x/ecocredit/migrations/v5/state_test.go

+
+	enrollment3, err := state.ProjectEnrollmentTable().Get(ctx, projKey3, clsKey2)
+	require.NoError(t, err)
+	require.Equal(t, projKey3, enrollment3.ProjectKey)


Suggested change

require.Equal(t, projKey3, enrollment3.ProjectKey)

require.Equal(t, projKey3, enrollment3.ProjectKey)

require.Equal(t, clsKey2, enrollment3.ClassKey)

clevinson · 2024-08-16T04:46:03Z

x/ecocredit/server/testsuite/genesis.go

+		{Id: "C01-001", Admin: sdk.AccAddress("addr1"), Jurisdiction: "AQ", Metadata: "metadata"},
+		{Id: "C01-002", Admin: sdk.AccAddress("addr2"), Jurisdiction: "AQ", Metadata: "metadata"},


Suggested change

{Id: "C01-001", Admin: sdk.AccAddress("addr1"), Jurisdiction: "AQ", Metadata: "metadata"},

{Id: "C01-002", Admin: sdk.AccAddress("addr2"), Jurisdiction: "AQ", Metadata: "metadata"},

{Id: "P001", Admin: sdk.AccAddress("addr1"), Jurisdiction: "AQ", Metadata: "metadata"},

{Id: "P002", Admin: sdk.AccAddress("addr2"), Jurisdiction: "AQ", Metadata: "metadata"},

clevinson · 2024-08-16T16:03:41Z

proto/regen/ecocredit/v1/tx.proto

-  // project with a matching reference id already exists within the scope of the
-  // credit class, otherwise a new project will be created.
+  // project with a matching reference id already exists, otherwise a new project
+  // will be created.


Why remove this text from the comment? Isn't it still true uniqueness of reference_id is only enforced within the scope of a credit class?

Co-authored-by: Cory <[email protected]>

aaronc · 2024-08-28T18:08:50Z

Here's my proposed solution for generating new batches IDs for existing projects:

if they are emitting a batch in their original credit class, then just use the old batch format, i.e. C01-001...
add an alt_batch_prefix field to Project that only can get set during the migration which gives them some alternate prefix to use if they emit a batch with a new credit class, that batch prefix could be something like L001 with L for legacy to distinguish them from the new P projects

How does that sound?

clevinson · 2024-08-28T18:45:06Z

So just to confirm,

If credit legacy project C01-001 registers then with a credit class C02, the first batch under this new credit class would look like C02-L001-20210101-20220101-001 ?

I'm trying to think if there are any queries we should be considering that may have weird or potentially confusing UX with this.

One that comes to mind - when querying for ProjectEnrollments, one would query by the original project ID (eg C01-001) to get a list of all enrolled credit classes, right?

This doesn't seem the prettiest but allowing queries by the alt project id also seems confusing.

The only other option I can think of is preventing legacy projects from enrolling in new credit classes. Personally this path seems cleanest to me, but I'm happy to default to whichever approach you think is best.

Assuming we do want to allow new enrollments from legacy projects, I think the proposal above makes sense and would support it.

paul121 · 2024-08-28T21:18:39Z

The only other option I can think of is preventing legacy projects from enrolling in new credit classes. Personally this path seems cleanest to me, but I'm happy to default to whichever approach you think is best.

I think I would prefer this as a solution as well.

Assuming we do want to allow new enrollments from legacy projects, I think the proposal above makes sense and would support it.

Agree

aaronc and others added 28 commits January 30, 2024 11:27

feat(x/ecocredit): independent project API

89e7630

feat(x/ecocredit): independent project API

c54af2a

fixes and proto-gen

d98b364

updates

78d71e1

add credit class to batch

cf50830

Merge branch 'main' of github.com:regen-network/regen-ledger into aar…

0139192

…onc/independent-projects

update to multiple credit classes per project

4a7370e

updates

f4146a3

add queries

864c63b

add queries

7f0af0d

add events

f6d3f93

Update proto/regen/ecocredit/v1/tx.proto

041af55

Co-authored-by: Cory <[email protected]>

Update proto/regen/ecocredit/v1/tx.proto

ea41f12

Co-authored-by: Cory <[email protected]>

address review comments

c3c15cd

proto-gen

d13c13c

fix build errors

809c089

fix tests with msg stubs

fe74b46

add msg_create_unregistered_project.feature

1798de5

udpates

b571c1f

update design

ad6b522

docs

a03d165

fixes

4c81f78

Merge branch 'aaronc/independent-projects' into aaronc/independent-pr…

89e986f

…ojects-impl

tests

222a6af

tests

d5feae9

tests and validation

4921fbe

add project creation fee

84ac1a4

Merge branch 'aaronc/independent-projects' into aaronc/independent-pr…

5faa1ae

…ojects-impl

aaronc changed the title ~~feat(x/ecocredit):c independent projects~~ feat(x/ecocredit): implement independent projects Feb 13, 2024

WIP

72f1d54

paul121 reviewed Jul 5, 2024

View reviewed changes

x/ecocredit/base/client/tx.go Outdated Show resolved Hide resolved

paul121 reviewed Jul 5, 2024

View reviewed changes

x/ecocredit/base/client/tx.go Outdated Show resolved Hide resolved

x/ecocredit/base/client/tx.go Outdated Show resolved Hide resolved

x/ecocredit/base/client/tx.go Outdated Show resolved Hide resolved

x/ecocredit/client/tx.go Show resolved Hide resolved

aaronc and others added 11 commits July 9, 2024 12:07

Update x/ecocredit/base/client/query.go

146ffb7

Co-authored-by: Paul Weidner <[email protected]>

Update x/ecocredit/base/client/query.go

9c208b4

Co-authored-by: Paul Weidner <[email protected]>

Update x/ecocredit/base/client/tx.go

27700ae

Co-authored-by: Paul Weidner <[email protected]>

Update x/ecocredit/base/client/tx.go

171c661

Co-authored-by: Paul Weidner <[email protected]>

Update x/ecocredit/base/client/tx.go

5687db8

Co-authored-by: Paul Weidner <[email protected]>

Update x/ecocredit/base/client/tx.go

92ceeec

Co-authored-by: Paul Weidner <[email protected]>

Update x/ecocredit/base/client/tx.go

9f9f9cc

Co-authored-by: Paul Weidner <[email protected]>

Update x/ecocredit/client/tx.go

3a5c295

Co-authored-by: Paul Weidner <[email protected]>

wire up missing cmd's

2de0250

fix example batch json

d25eebf

improve error messages

5146a89

allow MsgUpdateProjectEnrollment to use status UNSPECIFIED

fd96f71

paul121 reviewed Jul 10, 2024

View reviewed changes

x/ecocredit/base/client/tx.go Show resolved Hide resolved

aaronc and others added 2 commits July 10, 2024 17:43

fix integration test

c4b8c76

Remove classId from TestTxCreateProjectCmd

1216bbd

paul121 approved these changes Jul 21, 2024

View reviewed changes

paul121 reviewed Jul 21, 2024

View reviewed changes

x/ecocredit/base/keeper/msg_create_project.go Outdated Show resolved Hide resolved

Update x/ecocredit/base/keeper/msg_create_project.go

26b1f22

Co-authored-by: Paul Weidner <[email protected]>

clevinson reviewed Aug 20, 2024

View reviewed changes

aaronc and others added 3 commits August 20, 2024 10:46

Update proto/regen/ecocredit/v1/query.proto

f1968c5

Co-authored-by: Cory <[email protected]>

Update proto/regen/ecocredit/v1/query.proto

dc4f235

Co-authored-by: Cory <[email protected]>

Update x/ecocredit/base/client/tx.go

75a7dfe

Co-authored-by: Cory <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x/ecocredit)!: implement independent projects #2167

feat(x/ecocredit)!: implement independent projects #2167

aaronc commented Feb 13, 2024 •

edited

Loading

aaronc commented Jul 9, 2024

paul121 commented Jul 9, 2024

aaronc commented Jul 10, 2024

paul121 left a comment

paul121 left a comment

clevinson left a comment

clevinson Aug 13, 2024

clevinson Aug 13, 2024

clevinson Aug 15, 2024

clevinson Aug 15, 2024

clevinson Aug 15, 2024

clevinson Aug 16, 2024

clevinson Aug 16, 2024

aaronc commented Aug 28, 2024

clevinson commented Aug 28, 2024

paul121 commented Aug 28, 2024

	Then expect batch sequence with project id "P001" and next sequence "1"
	Then expect batch sequence with project id "P002" and next sequence "1"

	require.Equal(t, projKey3, enrollment3.ProjectKey)
	require.Equal(t, projKey3, enrollment3.ProjectKey)
	require.Equal(t, clsKey2, enrollment3.ClassKey)

		{Id: "C01-001", Admin: sdk.AccAddress("addr1"), Jurisdiction: "AQ", Metadata: "metadata"},
		{Id: "C01-002", Admin: sdk.AccAddress("addr2"), Jurisdiction: "AQ", Metadata: "metadata"},

feat(x/ecocredit)!: implement independent projects #2167

Are you sure you want to change the base?

feat(x/ecocredit)!: implement independent projects #2167

Conversation

aaronc commented Feb 13, 2024 • edited Loading

Description

Author Checklist

Reviewers Checklist

aaronc commented Jul 9, 2024

paul121 commented Jul 9, 2024

aaronc commented Jul 10, 2024

paul121 left a comment

Choose a reason for hiding this comment

paul121 left a comment

Choose a reason for hiding this comment

clevinson left a comment

Choose a reason for hiding this comment

clevinson Aug 13, 2024

Choose a reason for hiding this comment

clevinson Aug 13, 2024

Choose a reason for hiding this comment

clevinson Aug 15, 2024

Choose a reason for hiding this comment

clevinson Aug 15, 2024

Choose a reason for hiding this comment

clevinson Aug 15, 2024

Choose a reason for hiding this comment

clevinson Aug 16, 2024

Choose a reason for hiding this comment

clevinson Aug 16, 2024

Choose a reason for hiding this comment

aaronc commented Aug 28, 2024

clevinson commented Aug 28, 2024

paul121 commented Aug 28, 2024

aaronc commented Feb 13, 2024 •

edited

Loading